Skip to content

Don't save deleted records into data_store if mvcc is not enabled on RangePartition #104

Merged
lzxddz merged 1 commit intomainfrom
update-putall
Oct 11, 2025
Merged

Don't save deleted records into data_store if mvcc is not enabled on RangePartition #104
lzxddz merged 1 commit intomainfrom
update-putall

Conversation

@lzxddz
Copy link
Collaborator

@lzxddz lzxddz commented Oct 10, 2025 •
edited
Loading

Related PR:
eloqdata/eloqsql#136
eloqdata/eloqdoc#231
eloqdata/eloqkv#215

Summary by CodeRabbit

  • Bug Fixes
    • Corrected handling of range-partitioned deletes when MVCC is disabled, emitting DELETE operations for accurate deletion semantics and potential storage savings.
  • Refactor
    • Streamlined write batching by removing unnecessary bookkeeping to reduce overhead and improve efficiency.
  • Chores
    • Suppressed previously verbose debug logging during scan operations to reduce log noise without affecting behavior.

@lzxddz lzxddz linked an issue Oct 10, 2025 that may be closed by this pull request
[Feature] PutAll() should not save deleted records into data_store if mvcc is not enabled on RangePartition. #23
Closed
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 10, 2025 •
edited
Loading

Walkthrough

The client removed an unused per-partition counter, added an MVCC-enabled check during range-partition batching, and changed delete handling to emit DELETE when MVCC is off. Separately, a RocksDB scan debug log line was commented out, with no control-flow modifications.

Changes

Cohort / File(s) Summary
Client batching and MVCC gating
data_store_service_client.cpp		 Removed partition_record_cnt usage in PutAll; added local MVCC flag from Sharder::GetLocalCcShards()->EnableMvcc() in PrepareRangePartitionBatches; for range-partitioned deletes, emit DELETE when MVCC is disabled, otherwise emit PUT.
RocksDB logging tweak
eloq_data_store_service/rocksdb_data_store_common.cpp		 Commented out a debug log in RocksDBDataStoreCommon::ScanNext, disabling the log without changing behavior.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Caller
  participant Client as DataStoreServiceClient
  participant Sharder as txservice::Sharder
  participant Store as DataStore

  Caller->>Client: PrepareRangePartitionBatches(records)
  Client->>Sharder: GetLocalCcShards().EnableMvcc()
  Sharder-->>Client: mvccEnabled (bool)

  loop For each record
    alt record.markedDeleted
      alt mvccEnabled == false
        Note over Client: Choose DELETE for delete-marked record
        Client->>Store: Write(DELETE, key)
      else mvccEnabled == true
        Note over Client: Use PUT to honor MVCC tombstoning
        Client->>Store: Write(PUT, key, value)
      end
    else not deleted
      Client->>Store: Write(PUT, key, value)
    end
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Poem

In clovered code, I twitch my nose,
DELETE or PUT? The logic knows.
MVCC on—tombs we trust;
MVCC off—DELETE’s a must.
Logs grow quiet, counters light—
A nimble hop, the batches write. 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title clearly highlights the primary change of altering deletion behavior for range partitions when MVCC is disabled by stating that deleted records will not be persisted to the data store, which aligns with the core update in the changeset.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch update-putall
📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between db72040 and cea8902.

📒 Files selected for processing (2)
  • data_store_service_client.cpp (2 hunks)
  • eloq_data_store_service/rocksdb_data_store_common.cpp (1 hunks)
🔇 Additional comments (3)
eloq_data_store_service/rocksdb_data_store_common.cpp (1)

705-706: LGTM! Debug log appropriately disabled.

Commenting out the debug log reduces noise without affecting functionality.

data_store_service_client.cpp (2)

4310-4311: Verify MVCC flag is stable during batch processing.

The enabled_mvcc flag is queried once but used across multiple batch operations. Ensure that MVCC state cannot change mid-operation, as this could lead to inconsistent delete handling across batches.

4336-4343: Verify MVCC support for hash partitions in PreparePartitionBatches
data_store_service_client.cpp@4215 always issues DELETE for deleted records—confirm whether hash‐partitioned tables support MVCC. If they do, guard DELETE with !enabled_mvcc as in the range-partitioned logic; if not, add a comment documenting why hash and range partitions differ.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@lzxddz lzxddz merged commit b3e6590 into main Oct 11, 2025
1 check passed
@lzxddz lzxddz deleted the update-putall branch October 11, 2025 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@xiexiaoy xiexiaoy xiexiaoy approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Feature] PutAll() should not save deleted records into data_store if mvcc is not enabled on RangePartition.

2 participants

@lzxddz @xiexiaoy